SearchTrackingOptions.Clone() sets SearchTerms via the public setter. If SearchTerms was never set (it can be null today and Render() will simply omit it), the setter will throw due to Ensure.IsNotNullOrEmpty. This makes cloning (and anything that clones SearchOptions.Tracking) fail unexpectedly. Consider cloning the backing field directly or only assigning SearchTerms when it is non-null/non-empty so Clone() preserves current semantics.

Updated.

The XML docs for HasAncestor say the inner operator is executed "at the root", but this operator is executed in the ancestor context (while HasRoot is the one that moves to the root). Please update the param description to avoid misleading API consumers.

Fixed.

I'm not sure if I like the idea to alter user's provided options silently. I know it's a clone. But may be we should simply document it "if your search request uses returnScope, make sure to set ReturnStoredSource to true." BTW, why do we need to set it to true? Does it described somewhere?

Also am I right to think that we need all this clone-and-update stuff, only to use in like a 10 lines of code under? May be we can have a local variable instead?

The reason it gets automatically set is that, conceptually, returnScope is partially an enhancement to ReturnStoredSource. So if you use returnScope, then you are implicitly using ReturnStoredSource. So if we let the server throw, we're basically saying, "We know you're using stored source, because you're using returnScope, but you didn't set the other flag to tell us this, so we're now going to fail." This is generally bad API design.

On the other hand, if it were reasonable, conceptually, to choose different values here, then it would be appropriate to pass whatever value is set through to the server and let it decide if the combination is valid or not. We can still do this, it would just be slightly annoying to users specifying essentially the same thing twice.

Since it's not clear which approach we should take here, I'll switch to the second approach, which is less code for us, and isn't a breaking change if, somehow, both are allowed in the future.

The clone is needed to avoid mutating the settings that the application has a reference to, which would be very bad form. (Hence settings are very often immutable.) I don't follow what you mean about using a local variable.

About local variable. What if instead of clone and update options approach we will go something like:

var shouldReturnStoredSource = options.ReturnStoredSource;
if (returnScope != null)
{
   shouldReturnStoredSource = true;
}

... skipped lines until 1490 where we actually render the value.
renderedSearchDefinition.Add("returnStoredSource", shouldReturnStoredSource, shouldReturnStoredSource);

This way we do not need to clone, but we will include the returnStoredSource automatically.

Yeah, we could do that in this case. But since I changed the code here, we don't need to clone anymore. Nevertheless, in general settings should always be immutable or have a way of cloning, because modifying base settings without clobbering the base is a commonly needed pattern. Ideally, settings would be immutable, and hence we wouldn't be having this discussion.